Skip to content

feat: add OnStart hook for synchronous startup jobs #2079

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

HeerakKashyap
Copy link
Contributor

@HeerakKashyap HeerakKashyap commented Jul 19, 2025

Description:

  • Implements an OnStart hook for the GoFr framework, allowing users to register synchronous startup jobs that execute before the server starts.
  • The hook receives a fully initialized *gofr.Context with all DI-managed services, as requested by maintainers.
  • Removes any previous custom context logic and follows the GoFr way for dependency injection.
  • Updates documentation and provides usage examples.
  • Includes proof of working feature (see attached screenshots).

Testing:

  • Verified that OnStart hooks can access DI-managed services (e.g., SQL, Redis).
  • All relevant tests pass with MySQL and Redis running in Docker.

Fixes #1833


Let me know if you want to add or change anything, or if you need help attaching screenshots or filling out the checklist!

Copy link
Member

@Umang01-hash Umang01-hash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • There are no tests added for the newly added code.
  • No documentation updates for the new feature.
  • We have following code quality/linter issues in your PR:
Screenshot 2025-07-21 at 4 33 49 PM

Please resolve them along with other review comments.
Thankyou

@HeerakKashyap
Copy link
Contributor Author

HeerakKashyap commented Jul 21, 2025

  • There are no tests added for the newly added code.
  • No documentation updates for the new feature.
  • We have following code quality/linter issues in your PR:
Screenshot 2025-07-21 at 4 33 49 PM Please resolve them along with other review comments. Thankyou

should i update iin readme.md , it would be okay to do so right ? @Umang01-hash

@Umang01-hash
Copy link
Member

  • There are no tests added for the newly added code.
  • No documentation updates for the new feature.
  • We have following code quality/linter issues in your PR:
Screenshot 2025-07-21 at 4 33 49 PM Please resolve them along with other review comments. Thankyou

should i update iin readme.md , it would be okay to do so right ? @Umang01-hash

@HeerakKashyap I think we can update the existing docs or maybe create a new one for the same. Both can work depending upon the content we are sharing.

- Move OnStart logic to a dedicated, testable method.
- Fix all linter issues (deep-exit and cyclomatic complexity).
- Add unit tests for success and error cases.
- Add comprehensive documentation with a realistic example.
- Update the example app to demonstrate a real-world use case.
@HeerakKashyap HeerakKashyap force-pushed the fix/onstart-di-context branch from 57c4743 to 8526959 Compare July 21, 2025 14:38
@HeerakKashyap
Copy link
Contributor Author

HeerakKashyap commented Jul 21, 2025

ss for testing Test case are working fine @Umang01-hash @ccoVeille

@HeerakKashyap HeerakKashyap changed the title feat: add OnStart hook for synchronous startup jobs (closes #1833) feat: add OnStart hook for synchronous startup jobs Jul 22, 2025
@HeerakKashyap
Copy link
Contributor Author

kidnly have a look @ccoVeille @Umang01-hash

@ccoVeille
Copy link
Contributor

The code is in, but I would prefer if the context was not forged twice.

Can't we create the gofr.Context before calling startup hooks, then pass it through all the layers as argument up to the place where the gofr.Context is created now?

Or would this implies breaking changes,? Or simply too much changes?

Please note, it doesn't necessary have to be addressed in this PR.

@HeerakKashyap
Copy link
Contributor Author

The code is in, but I would prefer if the context was not forged twice.

Can't we create the gofr.Context before calling startup hooks, then pass it through all the layers as argument up to the place where the gofr.Context is created now?

Or would this implies breaking changes,? Or simply too much changes?

Please note, it doesn't necessary have to be addressed in this PR.

Thank you for the suggestion Sir! @ccoVeille

I agree that creating the gofr.Context only once and passing it through would be cleaner and more efficient.
However, implementing this would require significant refactoring and could potentially introduce breaking changes, especially if other parts of the framework expect to construct their own context.

For this PR, I’ll keep the current approach to avoid introducing breaking changes, but I think this is a great idea for future improvement and can be considered in a dedicated refactor.

@HeerakKashyap
Copy link
Contributor Author

@Umang01-hash sir, kindly run the workflows and let me know is there now any linter errors etc.

@Umang01-hash
Copy link
Member

Hey @HeerakKashyap only a few things:

  • Changes in go.work.sum file needs to be reverted.
  • We dont need a newContextForHooks methods, we can use exitisting methods to create new context.
  • A lot of linter errors are there in the code we need to resolve them.

Please do the above changes and we should be ready to go with your PR.

HeerakKashyap added 3 commits July 23, 2025 22:56
- Fix dynamic error definition
- Fix unused parameters with underscore
- Use t.Context() instead of context.Background()
- Use assert.NoError instead of assert.Nil
- Fix cuddled return statement in newContextForHooks
- Fix cuddled assignment in OnStart test
- Remove os.Exit call to resolve deep-exit error
@HeerakKashyap HeerakKashyap force-pushed the fix/onstart-di-context branch from f4882d6 to 9e8bcdf Compare July 23, 2025 17:52
@HeerakKashyap
Copy link
Contributor Author

@coolwednesday The distinction between context.Canceled and other errors is :

context.Canceled (graceful shutdown):

  • User initiated shutdown (Ctrl+C, SIGTERM)

  • Application should exit cleanly without error messages

  • No need to log as "Startup failed" since it's intentional

    Other errors (startup failure):

  • Actual problems during startup (DB connection failed, config errors, etc.)

  • Should log as "Startup failed" for debugging

  • Application should exit with error status

Example scenarios:

  • context.Canceled: User hits Ctrl+C during startup → graceful exit
  • Other error: Redis connection fails → log error and exit with failure status

I believe this allows users to distinguish between intentional shutdowns and actual startup problems.

@coolwednesday
Copy link
Member

@HeerakKashyap , Please address all the review comments, so that we can close this ASAP.

@HeerakKashyap
Copy link
Contributor Author

HeerakKashyap commented Jul 31, 2025

@HeerakKashyap , Please address all the review comments, so that we can close this ASAP.

I've given replies above for the same , so should i do all the changes given by the maintainers ?

@coolwednesday
Copy link
Member

@coolwednesday The distinction between context.Canceled and other errors is :

context.Canceled (graceful shutdown):

  • User initiated shutdown (Ctrl+C, SIGTERM)
  • Application should exit cleanly without error messages
  • No need to log as "Startup failed" since it's intentional
    Other errors (startup failure):
  • Actual problems during startup (DB connection failed, config errors, etc.)
  • Should log as "Startup failed" for debugging
  • Application should exit with error status

Example scenarios:

  • context.Canceled: User hits Ctrl+C during startup → graceful exit
  • Other error: Redis connection fails → log error and exit with failure status

I believe this allows users to distinguish between intentional shutdowns and actual startup problems.

If we are thinking about graceful exit. It should happen in all cases. That is all the DB/Stream Connections started should be closed with graceful exit. Keep Context.Canceled only.

@coolwednesday
Copy link
Member

@HeerakKashyap , Please address all the review comments, so that we can close this ASAP.

I've given replies above for the same , so should i do all the changes given by the maintainers ?

Yes Please address them.

coolwednesday
coolwednesday previously approved these changes Aug 4, 2025
Copy link
Member

@coolwednesday coolwednesday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some documentation fix. Rest LGTM.

Thanks !

Umang01-hash
Umang01-hash previously approved these changes Aug 4, 2025
coolwednesday
coolwednesday previously approved these changes Aug 4, 2025
@aryanmehrotra aryanmehrotra merged commit 58fdefb into gofr-dev:development Aug 4, 2025
14 checks passed
aryanmehrotra added a commit that referenced this pull request Aug 5, 2025
* Fix/pubsub migration get last version (#2119)

* add chaining of next migrator in getLastVersion method of pubsub.go

* fix linter

* feat: add OnStart hook for synchronous startup jobs (#2079)

* feat: improve OnStart hook with DI context, update docs, and ensure code quality

* refactor: improve OnStart hook context, move logic to method, and test DI access

* feat(onstart): implement all reviewer feedback

- Move OnStart logic to a dedicated, testable method.
- Fix all linter issues (deep-exit and cyclomatic complexity).
- Add unit tests for success and error cases.
- Add comprehensive documentation with a realistic example.
- Update the example app to demonstrate a real-world use case.

* fix: improve startup error handling for OnStart hooks (graceful shutdown on context cancel)

* fix: address linter errors in OnStart tests

- Fix dynamic error definition
- Fix unused parameters with underscore
- Use t.Context() instead of context.Background()
- Use assert.NoError instead of assert.Nil

* fix: remove os.Exit call to resolve deep-exit linter error

* fix: resolve remaining linter errors

- Fix cuddled return statement in newContextForHooks
- Fix cuddled assignment in OnStart test
- Remove os.Exit call to resolve deep-exit error

* refactor: remove newContextForHooks and reuse existing newContext function

* fix: resolve linting errors and refactor Run() method for better maintainability

- Fix err113: Replace dynamic error creation with static error in TestApp_OnStart
- Fix gocyclo: Refactor Run() method by extracting helper methods to reduce cyclomatic complexity
- Fix ws1: Correct whitespace issues in gofr_test.go and run.go
- Add helper methods: handleStartupHooks, startShutdownHandler, startTelemetryIfEnabled, startAllServers
- Improve code organization and readability while maintaining functionality

* fix: resolve remaining linting issues in TestApp_OnStart

- Fix err113: Move error variable to package level to avoid dynamic error creation
- Fix error-naming: Rename hookFailedErr to errHookFailed following Go naming conventions
- Fix whitespace: Remove unnecessary blank line before app assignment
- All linting issues in modified files are now resolved

* fix: resolve godot and wsl linter errors in run.go and gofr_test.go

* Update go.work.sum

* fix: restore comments as requested by reviewer

* Update run.go

* Update run.go

* fix: address reviewer comments - add godoc comment to OnStart method and move startup-hooks.md to proper directory structure

* fix: restore metrics server comments and ensure OnStart hook runs before route registration

* fix: implement proper context usage in runOnStartHooks and fix example API usage

* Update go.work.sum

* fix: correct spelling of 'canceled' in comment

* minor documentation fix

* revert go.sum changes

* Update pkg/gofr/gofr_test.go

Co-authored-by: ccoVeille <[email protected]>

* revert go.work.sum changes

---------

Co-authored-by: HeerakKashyap <[email protected]>
Co-authored-by: Umang Mundhra <[email protected]>
Co-authored-by: coolwednesday <[email protected]>
Co-authored-by: Divya Darshana <[email protected]>
Co-authored-by: ccoVeille <[email protected]>

* build(deps): bump github.com/prometheus/client_golang

Bumps [github.com/prometheus/client_golang](https://github.com/prometheus/client_golang) from 1.22.0 to 1.23.0.
- [Release notes](https://github.com/prometheus/client_golang/releases)
- [Changelog](https://github.com/prometheus/client_golang/blob/main/CHANGELOG.md)
- [Commits](prometheus/client_golang@v1.22.0...v1.23.0)

---
updated-dependencies:
- dependency-name: github.com/prometheus/client_golang
  dependency-version: 1.23.0
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

* build(deps): bump go.opentelemetry.io/otel/exporters/prometheus

Bumps [go.opentelemetry.io/otel/exporters/prometheus](https://github.com/open-telemetry/opentelemetry-go) from 0.59.0 to 0.59.1.
- [Release notes](https://github.com/open-telemetry/opentelemetry-go/releases)
- [Changelog](https://github.com/open-telemetry/opentelemetry-go/blob/main/CHANGELOG.md)
- [Commits](open-telemetry/opentelemetry-go@exporters/prometheus/v0.59.0...exporters/prometheus/v0.59.1)

---
updated-dependencies:
- dependency-name: go.opentelemetry.io/otel/exporters/prometheus
  dependency-version: 0.59.1
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

* build(deps): bump gofr.dev in /examples/using-add-filestore

Bumps [gofr.dev](https://github.com/gofr-dev/gofr) from 1.42.4 to 1.42.5.
- [Release notes](https://github.com/gofr-dev/gofr/releases)
- [Commits](v1.42.4...v1.42.5)

---
updated-dependencies:
- dependency-name: gofr.dev
  dependency-version: 1.42.5
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

* build(deps): bump github.com/ClickHouse/clickhouse-go/v2

Bumps [github.com/ClickHouse/clickhouse-go/v2](https://github.com/ClickHouse/clickhouse-go) from 2.39.0 to 2.40.1.
- [Release notes](https://github.com/ClickHouse/clickhouse-go/releases)
- [Changelog](https://github.com/ClickHouse/clickhouse-go/blob/main/CHANGELOG.md)
- [Commits](ClickHouse/clickhouse-go@v2.39.0...v2.40.1)

---
updated-dependencies:
- dependency-name: github.com/ClickHouse/clickhouse-go/v2
  dependency-version: 2.40.1
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

* build(deps): bump crate-ci/typos in the actions group

Bumps the actions group with 1 update: [crate-ci/typos](https://github.com/crate-ci/typos).


Updates `crate-ci/typos` from 1.34.0 to 1.35.1
- [Release notes](https://github.com/crate-ci/typos/releases)
- [Changelog](https://github.com/crate-ci/typos/blob/master/CHANGELOG.md)
- [Commits](crate-ci/typos@v1.34.0...v1.35.1)

---
updated-dependencies:
- dependency-name: crate-ci/typos
  dependency-version: 1.35.1
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: actions
...

Signed-off-by: dependabot[bot] <[email protected]>

* build(deps): bump github.com/prometheus/client_golang

Bumps [github.com/prometheus/client_golang](https://github.com/prometheus/client_golang) from 1.22.0 to 1.23.0.
- [Release notes](https://github.com/prometheus/client_golang/releases)
- [Changelog](https://github.com/prometheus/client_golang/blob/main/CHANGELOG.md)
- [Commits](prometheus/client_golang@v1.22.0...v1.23.0)

---
updated-dependencies:
- dependency-name: github.com/prometheus/client_golang
  dependency-version: 1.23.0
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

* build(deps): bump google.golang.org/api from 0.243.0 to 0.244.0

Bumps [google.golang.org/api](https://github.com/googleapis/google-api-go-client) from 0.243.0 to 0.244.0.
- [Release notes](https://github.com/googleapis/google-api-go-client/releases)
- [Changelog](https://github.com/googleapis/google-api-go-client/blob/main/CHANGES.md)
- [Commits](googleapis/google-api-go-client@v0.243.0...v0.244.0)

---
updated-dependencies:
- dependency-name: google.golang.org/api
  dependency-version: 0.244.0
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

* build(deps): bump modernc.org/sqlite from 1.38.1 to 1.38.2

Bumps [modernc.org/sqlite](https://gitlab.com/cznic/sqlite) from 1.38.1 to 1.38.2.
- [Commits](https://gitlab.com/cznic/sqlite/compare/v1.38.1...v1.38.2)

---
updated-dependencies:
- dependency-name: modernc.org/sqlite
  dependency-version: 1.38.2
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

* build(deps): bump github.com/golang-jwt/jwt/v5 from 5.2.3 to 5.3.0

Bumps [github.com/golang-jwt/jwt/v5](https://github.com/golang-jwt/jwt) from 5.2.3 to 5.3.0.
- [Release notes](https://github.com/golang-jwt/jwt/releases)
- [Changelog](https://github.com/golang-jwt/jwt/blob/main/VERSION_HISTORY.md)
- [Commits](golang-jwt/jwt@v5.2.3...v5.3.0)

---
updated-dependencies:
- dependency-name: github.com/golang-jwt/jwt/v5
  dependency-version: 5.3.0
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

* Merge pull request #2135 from gofr-dev/fix-docs

* update release version to v1.43.0

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: HootingConjuror <[email protected]>
Co-authored-by: HeerakKashyap <[email protected]>
Co-authored-by: coolwednesday <[email protected]>
Co-authored-by: Divya Darshana <[email protected]>
Co-authored-by: ccoVeille <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants